Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow for injection of new tabs on edit page #4440

Merged
merged 1 commit into from
Jul 16, 2020
Merged

Allow for injection of new tabs on edit page #4440

merged 1 commit into from
Jul 16, 2020

Conversation

cjcolvar
Copy link
Member

@cjcolvar cjcolvar commented Jul 14, 2020

This commit adds a new code seam allowing for the modification of tab
ordering and the addition (or removal) of tabs on a form by form basis.
The default tab order is moved out of the guts4form view partial and
into a helper.

To modify the default list of tabs in an application override the form_tabs_for helper method and make sure it gets loaded
after Hyrax::HyraxHelperBehavoir (by default included in app/helpers/hyrax_helper.rb). For example:

module WorksHelper
  def form_tabs_for(form:)
    super + ["my_new_tab"]
  end
end

The share tab has not been included in the default tab order because it
wasn't in the view partial. Future work is to simplify the guts4form
partial so share can be treated the same as the other tabs and included
in the default list.

@samvera/hyrax-code-reviewers

@@ -1,5 +1,5 @@
<% # we will yield to content_for for each tab, e.g. :files_tab %>
<% tabs ||= %w[metadata files relationships] # default tab order %>
<% tabs ||= f.object.tabs # default tab order %>
Copy link
Contributor

@no-reply no-reply Jul 14, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i'm thinking about whether the Form object is the best home for this.

i know the older WorkForm serves in some ways as both a configuration object and as a data object/presenter. my experience has been that this has been something of a pain point (e.g. i think it's the main reason we haven't been able to make forms repopulate on failed submissions). i've been trying, only somewhat successfully, to limit our repetition of this pattern in ResourceForm. we still have primary/secondary terms, for instance, which are purely display configuration.

so i guess i'm open to this, but also wondering whether there's any downside to using a simple helper? <% form_tabs_for(form: f.object) %> with an implementation something like:

def form_tabs_for(form:)
  %w[metadata files relationships]
end

honestly, this could also be a viable pattern for moving away from #primary/secondary_terms?


(also, what's going on with tabs ||= here?! this should be a separate PR, but let's try just dropping that?)

Copy link
Contributor

@no-reply no-reply Jul 14, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah. tabs is being passed in by the caller in some places. maybe a good idea to just have the caller pass it in always, but definitely out of scope.

on the other hand, this does mean we need to consider what other forms would need to implement this new method, and probably provide a fall back in case applications provide their own form objects.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not entirely following. Are you suggesting using a helper or passing the tabs in or both?
My ultimate goal is having an engine plugin that can easily inject a new tab for the create/edit form without having to do any overrides and have this be on a per work-type basis. How would a plugin add a new tab if an application already has the helper implemented? With the form approach it seems straightforward with something like:

module Plugin
  module FormBehavior
    included do
      self.tabs += ['newtab']
    end
  end
end

Although there might be timing issues with that. I'm open to any approaches you think I should try.

Copy link
Contributor

@no-reply no-reply Jul 15, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sorry, i'll rephrase hopefully with more clarity.

what i understand to be happening now is:

  • the guts4form partial accepts a tabs: [:blah] argument;
  • if that argument is not passed in by render, we use %w[metadata files relationships] as the default;

the current state of this PR changes the second point to: use f.object.tabs as the default. that seems fine as far as it goes, but:

  • i'm wondering whether putting this knowledge in the Form has advantages for applications over a helper which they could override.
    • i've been otherwise trying to keep the new ResourceForm objects as thin as backward compatibility allows, but i'm not strongly committed to that. i can say more about this if it's helpful.
    • i'm suggesting passing the form object into the helper so apps can choose implementations that switch tabs based on the form class, or other attributes of the form object (maybe this isn't needed?)
  • i think the branching behavior in "happening now" above isn't great, and we should always have render pass in tabs: [:something], but that's probably out of scope for your goals and i don't want to foist it on you.

@cjcolvar cjcolvar force-pushed the form-tabs branch 3 times, most recently from c719ea8 to 8707bff Compare July 15, 2020 19:45
This commit adds a new code seam allowing for the modification of tab
ordering and the addition (or removal) of tabs on a form by form basis.
The default tab order is moved out of the guts4form view partial and
into a helper.

To modify the default list of tabs in an application override the `form_tabs_for` helper method and make sure it gets loaded
after Hyrax::HyraxHelperBehavoir (by default included in app/helpers/hyrax_helper.rb). For example:
```
module WorksHelper
  def form_tabs_for(form:)
    super + ["my_new_tab"]
  end
end
```

The share tab has not been included in the default tab order because it
wasn't in the view partial.  Future work is to simplify the guts4form
partial so share can be treated the same as the other tabs and included
in the default list.
@cjcolvar
Copy link
Member Author

@no-reply I've switched over to the helper approach and added a note about how to override it in an application. Do you mind reviewing again when you have a chance?

@no-reply no-reply merged commit 5051e81 into master Jul 16, 2020
@jeremyf jeremyf deleted the form-tabs branch July 16, 2020 01:34
cjcolvar added a commit that referenced this pull request Jul 16, 2020
This commit adds a new code seam allowing for the modification of tab
ordering and the addition (or removal) of tabs on a form by form basis.
The default tab order is moved out of the guts4form view partial and
into a helper.

To modify the default list of tabs in an application override the `form_tabs_for` helper method and make sure it gets loaded
after Hyrax::HyraxHelperBehavoir (by default included in app/helpers/hyrax_helper.rb). For example:
```
module WorksHelper
  def form_tabs_for(form:)
    super + ["my_new_tab"]
  end
end
```

A deprecation warning was added for cases where the view calling
guts4form doesn't pass the tabs local param.

The share tab has not been included in the default tab order because it
wasn't in the view partial.  Future work is to simplify the guts4form
partial so share can be treated the same as the other tabs and included
in the default list.

Backport of #4440.
cjcolvar added a commit that referenced this pull request Jul 16, 2020
This commit adds a new code seam allowing for the modification of tab
ordering and the addition (or removal) of tabs on a form by form basis.
The default tab order is moved out of the guts4form view partial and
into a helper.

To modify the default list of tabs in an application override the `form_tabs_for` helper method and make sure it gets loaded
after Hyrax::HyraxHelperBehavoir (by default included in app/helpers/hyrax_helper.rb). For example:
```
module WorksHelper
  def form_tabs_for(form:)
    super + ["my_new_tab"]
  end
end
```

A deprecation warning was added for cases where the view calling
guts4form doesn't pass the tabs local param.

The share tab has not been included in the default tab order because it
wasn't in the view partial.  Future work is to simplify the guts4form
partial so share can be treated the same as the other tabs and included
in the default list.

Backport of #4440.
cjcolvar added a commit that referenced this pull request Jul 16, 2020
This commit adds a new code seam allowing for the modification of tab
ordering and the addition (or removal) of tabs on a form by form basis.
The default tab order is moved out of the guts4form view partial and
into a helper.

To modify the default list of tabs in an application override the `form_tabs_for` helper method and make sure it gets loaded
after Hyrax::HyraxHelperBehavoir (by default included in app/helpers/hyrax_helper.rb). For example:
```
module WorksHelper
  def form_tabs_for(form:)
    super + ["my_new_tab"]
  end
end
```

A deprecation warning was added for cases where the view calling
guts4form doesn't pass the tabs local param.

The share tab has not been included in the default tab order because it
wasn't in the view partial.  Future work is to simplify the guts4form
partial so share can be treated the same as the other tabs and included
in the default list.

Backport of #4440.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants